fix(v3): prevent SIGSEGV enumerating screens on display change (macOS)#5516
fix(v3): prevent SIGSEGV enumerating screens on display change (macOS)#5516flofreud wants to merge 5 commits into
Conversation
WalkthroughRefactors macOS screen handling to compute primary screen height once, change C getAllScreens to return a counted Screen* snapshot, and marshal AppKit screen enumeration onto the main thread to prevent SIGSEGV during display configuration changes. ChangesDarwin Screen Enumeration SIGSEGV Fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`ApplicationDidChangeScreenParameters` is dispatched on background goroutines (event_manager.go) and can fire several times in quick succession during a single display reconfiguration. `macosApp. processAndCacheScreens` enumerated `[NSScreen screens]` and messaged each `NSScreen` off the main thread, so a burst of events ran `getAllScreens` concurrently and crashed with a native SIGSEGV during cgo execution. As a hard signal it is not catchable by the dispatcher's `defer handlePanic()`. - Marshal the screen enumeration onto the main thread via `InvokeSync`, guarding against a deadlock when already on the main thread. Running on the main run loop also serialises the event burst so `[NSScreen screens]` is never touched concurrently. - Resolve the primary-screen height once per refresh instead of re-enumerating `[NSScreen screens]` inside `processScreen` for every screen (introduced in alpha.91, which multiplied the off-main-thread AppKit access in this path). - Return the screen count from `getAllScreens` so the Go side no longer reads the count separately via `GetNumScreens`, closing a TOCTOU that could over-read the malloc'd buffer when a display is added mid-refresh. Refs wailsapp#5117 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
flofreud
force-pushed
the
fix/v3-macos-screen-params-crash
branch
from
4b8327d to
7cad7ad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/pkg/application/screen_darwin.go (1)
113-126: ⚡ Quick winReuse the captured snapshot for primary height instead of re-enumerating.
getAllScreenssnapshots[NSScreen screens]at Line 114 to size the allocation, but Line 119 callsprimaryScreenHeight(), which enumerates[NSScreen screens]a second time. This both contradicts the "single snapshot" intent of the fix and means the primary height can come from a different snapshot than the count/loop. Sincescreens[0]is the primary screen (matchingisPrimary = (i == 0)), derive the height from the already-captured array.♻️ Derive primary height from the existing snapshot
if (outCount != NULL) { *outCount = (int)count; } - CGFloat primaryHeight = primaryScreenHeight(); + // Reuse the snapshot above instead of re-enumerating [NSScreen screens]; + // screens[0] is the primary screen (matches isPrimary = (i == 0) below). + CGFloat primaryHeight = count > 0 ? [[screens objectAtIndex:0] frame].size.height : 0; Screen* returnScreens = malloc(sizeof(Screen) * count);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/screen_darwin.go` around lines 113 - 126, getAllScreens currently snapshots NSArray *screens but then calls primaryScreenHeight(), which re-enumerates NSScreen; instead derive the primary height from the already-captured snapshot: after capturing screens (NSArray<NSScreen *> *screens), compute primaryHeight using the first element ([screens objectAtIndex:0])'s frame/visibleFrame height and pass that into processScreen for all items so the primary height is consistent with the snapshot; update getAllScreens to stop calling primaryScreenHeight() and use the height extracted from screens[0] (keep the existing processScreen and isPrimary = (i == 0) logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/pkg/application/screen_darwin.go`:
- Around line 113-126: getAllScreens currently snapshots NSArray *screens but
then calls primaryScreenHeight(), which re-enumerates NSScreen; instead derive
the primary height from the already-captured snapshot: after capturing screens
(NSArray<NSScreen *> *screens), compute primaryHeight using the first element
([screens objectAtIndex:0])'s frame/visibleFrame height and pass that into
processScreen for all items so the primary height is consistent with the
snapshot; update getAllScreens to stop calling primaryScreenHeight() and use the
height extracted from screens[0] (keep the existing processScreen and isPrimary
= (i == 0) logic).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7fc19a9-fd30-4730-9d6b-336c88e84d4f
📒 Files selected for processing (2)
v3/UNRELEASED_CHANGELOG.mdv3/pkg/application/screen_darwin.go
getAllScreens snapshotted [NSScreen screens] to size the allocation but then called primaryScreenHeight(), which enumerated [NSScreen screens] a second time — contradicting the single-snapshot intent and risking a primary height from a different snapshot than the count/loop. Derive it from screens[0] (already treated as primary via isPrimary = i == 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/pkg/application/screen_darwin.go (1)
261-272: 💤 Low valueConsider main-thread marshalling for remaining AppKit accessors.
getScreenForWindowandgetScreenForSystraycall C functions that access[NSScreen screens](viaprimaryScreenHeight()) without main-thread marshalling. If invoked from a background goroutine during a display reconfiguration, they could hit the same SIGSEGV the PR fixes forprocessAndCacheScreens. These paths are lower risk since they're typically called in response to user actions rather than system events, but you may want to add similar protection in a follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/pkg/application/screen_darwin.go` around lines 261 - 272, getScreenForWindow and getScreenForSystray call C functions that touch AppKit ([NSScreen screens]) and must be called on the main thread to avoid SIGSEGV; wrap the calls to C.getScreenForWindow and C.getWindowForSystray in the same main-thread marshalling used elsewhere (e.g., a runOnMain / runMainSync helper or the pattern used in processAndCacheScreens) so the C.get* calls and subsequent cScreenToScreen conversion run on the main thread, propagate any nil cScreen as an error, and return the Screen only after the main-thread call completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@v3/pkg/application/screen_darwin.go`:
- Around line 261-272: getScreenForWindow and getScreenForSystray call C
functions that touch AppKit ([NSScreen screens]) and must be called on the main
thread to avoid SIGSEGV; wrap the calls to C.getScreenForWindow and
C.getWindowForSystray in the same main-thread marshalling used elsewhere (e.g.,
a runOnMain / runMainSync helper or the pattern used in processAndCacheScreens)
so the C.get* calls and subsequent cScreenToScreen conversion run on the main
thread, propagate any nil cScreen as an error, and return the Screen only after
the main-thread call completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2c54d8d-1a7b-4bd3-ba6b-cbc4204ea9f8
📒 Files selected for processing (1)
v3/pkg/application/screen_darwin.go
|
@leaanthony I addressed the first CodeRabbit nit-pick but the one about Wails generally not considering main-thread marshalling observed by Claude and CodeRabbit should probably be a more focused effort later. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v3/UNRELEASED_CHANGELOG.md`:
- Line 27: Update the user-facing changelog text to use consistent US spelling:
replace “unmaximise” with “unmaximize” in the UNRELEASED_CHANGELOG.md entry that
reads "Fix minimum width/height constraints not enforced after window unmaximise
on Windows (`#4593`)"; ensure the rest of the line remains unchanged except for
that word so the entry reads "Fix minimum width/height constraints not enforced
after window unmaximize on Windows (`#4593`)".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f76df91-e4e5-45ad-acd9-8051603076ec
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md
| ## Fixed | ||
| <!-- Bug fixes --> | ||
| - Fix SIGSEGV when the display configuration changes (sleep/wake, monitor connect/disconnect). `ApplicationDidChangeScreenParameters` is dispatched on background goroutines and can fire repeatedly during a single reconfiguration, so `processAndCacheScreens` enumerated `[NSScreen screens]` concurrently off the main thread and crashed. Screen enumeration is now marshalled onto the main thread (which also serialises the event burst), the primary-screen height is resolved once per refresh instead of per screen, and `getAllScreens` returns its count to close a TOCTOU against the buffer size. (#5117) | ||
| - Fix minimum width/height constraints not enforced after window unmaximise on Windows (#4593) |
There was a problem hiding this comment.
Use consistent spelling in user-facing changelog text.
Line 27 uses “unmaximise”; consider “unmaximize” for consistency with common US spelling in release notes.
🧰 Tools
🪛 LanguageTool
[grammar] ~27-~27: Ensure spelling is correct
Context: ...t constraints not enforced after window unmaximise on Windows (#4593) - Fix mouse click-th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v3/UNRELEASED_CHANGELOG.md` at line 27, Update the user-facing changelog text
to use consistent US spelling: replace “unmaximise” with “unmaximize” in the
UNRELEASED_CHANGELOG.md entry that reads "Fix minimum width/height constraints
not enforced after window unmaximise on Windows (`#4593`)"; ensure the rest of the
line remains unchanged except for that word so the entry reads "Fix minimum
width/height constraints not enforced after window unmaximize on Windows
(`#4593`)".
2 participants
Description
On macOS, a v3 app crashes with a native
SIGSEGVoften when the displayconfiguration changes — sleep/wake, lid open/close, monitor connect/disconnect,
resolution or arrangement change.
After building with this fix the issue seems to be resolved.
Root cause
ApplicationDidChangeScreenParametersis dispatched on a background goroutine(
event_manager.goruns every application-event listener in its owngo func),and macOS emits the event several times in quick succession during a single
reconfiguration. The handler calls
macosApp.processAndCacheScreens, which calls[NSScreen screens]and messages eachNSScreen. AppKit is main-thread-only, soa burst of events runs
getAllScreensconcurrently, off the main thread,while the display graph is itself being reconfigured → use-after-free / message to
a freed object →
SIGSEGV. Because it is a native crash during cgo execution, thedispatcher's
defer handlePanic()cannot recover it and the process dies.Observed at crash time: three goroutines simultaneously inside
processAndCacheScreens, each spawned by a separatehandleApplicationEvent.The per-screen
[[NSScreen screens] firstObject]lookup added toprocessScreenin alpha.91 (#5117) multiplied the off-main-thread AppKit access in this path and
made the crash much easier to hit.
Fix
InvokeSync, guardingagainst a deadlock when already on the main thread. The main run loop also
serialises the burst of events, so
[NSScreen screens]is never accessedconcurrently.
re-enumerating
[NSScreen screens]insideprocessScreenfor every screen.getAllScreensso the Go side no longer readsthe count separately via
GetNumScreens— closing a TOCTOU where a displayadded between the two calls could make the loop over-read the
malloc'd buffer.How to test
go build ./pkg/application/andgo vet ./pkg/application/pass.Refs #5117
Summary by CodeRabbit